d/aws_prefix_list: fixes and enhancements #14109

roberth-k · 2020-07-09T10:08:16Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Contains bugfixes and enhancements to the aws_prefix_list data source discovered while implementing support for Managed Prefix Lists.

Users may be affected by the following changes in behavior:

Where no criteria have been provided to the data source, the data source currently produces an arbitrary prefix list. After this PR, it will raise an error.
Where a resource depends on the API ordering of CIDR blocks (such as by selecting cidr_blocks[0]), the result may now be a different CIDR block.

Release note for CHANGELOG:

ENHANCEMENTS:
- data-source/aws_prefix_list: The `cidr_blocks` attribute is now lexically sorted
- data-source/aws_prefix_list: Add documentation example about finding a prefix list by name

BUG FIXES:
- data-source/aws_prefix_list: Raise error when criteria match more than one prefix list
- data-source/aws_prefix_list: Specifying the `name` attribute no longer overwrites all filters

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS=-run=TestAccDataSourceAwsPrefixList_
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsPrefixList_ -timeout 120m
--- PASS: TestAccDataSourceAwsPrefixList_matchesTooMany (7.09s)
--- PASS: TestAccDataSourceAwsPrefixList_nameDoesNotOverrideFilter (7.10s)
--- PASS: TestAccDataSourceAwsPrefixList_basic (27.39s)
--- PASS: TestAccDataSourceAwsPrefixList_filter (28.37s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       29.752s
...

…tching multiple prefix lists

…x list by name

bflad

Hi @roberth-k 👋 Thank you for submitting this! Overall this is good stuff, but I marked this as breaking changes for now since we would want to hold off on purposefully introducing changes that effect practitioner experience until a major version update. We can split this up into fixing the name versus filter bug now and separately introducing the other changes.

Please reach out if you have any questions. 😄

bflad · 2020-12-03T18:54:04Z

aws/data_source_aws_prefix_list.go


 log.Printf("[DEBUG] Reading Prefix List: %s", req)
 resp, err := conn.DescribePrefixLists(req)
- if err != nil {
+ switch {


The predominate coding style for the codebase is if conditionals with early returns rather than bare switch conditionals. Can you please change this back rather than introducing a different coding style? Thanks!

bflad · 2020-12-03T18:58:13Z

aws/data_source_aws_prefix_list.go

+ case len(resp.PrefixLists) > 1:
+ return fmt.Errorf("more than one prefix list matched the given set of criteria")


While this is a perfectly valid change for consistency with most other data sources, we should only perform this change during a major version upgrade to align with practitioner versioning expectations, which the next one will be a few months away. It would be best to remove this change here and create a separate GitHub bug report so we can mark it for that future milestone. 👍 You're more than welcome to submit this change as well separately.

bflad · 2020-12-03T19:04:03Z

aws/data_source_aws_prefix_list.go

- for i, v := range pl.Cidrs {
- cidrs[i] = *v
+ cidrs := aws.StringValueSlice(pl.Cidrs)
+ sort.Strings(cidrs)


Similarly here with respect to introducing a breaking change which should be removed for now.

In this case though, if the ordering of the CIDR Blocks is not significant (which it likely is not), the cidr_blocks attribute should be changed to TypeSet instead of attempting to add ordering that is inconsistent with the semantics of the upstream API. Terraform configurations can and should use built in language support such as resource for_each for handling all elements of an attribute like this to avoid ordering concerns.

At some point in the future we may go back through many of the TypeList attributes and convert them to TypeSet to ensure that Terraform configurations are not depending on ordering where its not guaranteed or semantically correct.

bflad · 2020-12-03T19:08:26Z

aws/data_source_aws_prefix_list_test.go

+
+const testAccDataSourceAwsPrefixListConfig_nameDoesNotOverrideFilter = `
+data "aws_prefix_list" "test" {
+ name = "com.amazonaws.us-west-2.s3" 


Some of our newer codebase linting will likely pick this up on rebase/merge for not being AWS Region agnostic. Similar to the website example you added (👍 ) we should use the aws_region data source here so the configuration can be applied in anywhere. This also means we'll need to remove the hardcoded EC2 Prefix List identifier. Something like this should get you close:

data "aws_region" "current" {} data "aws_prefix_list" "dynamodb" { name = "com.amazonaws.${data.aws_region.current.name}.dynamodb" } data "aws_prefix_list" "test" { name = "com.amazonaws.${data.aws_region.current.name}.s3" filter { name = "prefix-list-id" values = [data.aws_prefix_list.dynamodb.id] } }

roberth-k · 2020-12-13T16:49:40Z

@bflad The managed prefix list data source has been spun off as #16738, and the fixes for the original data source at #16739.

ghost · 2021-01-12T17:10:00Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

roberth-k requested a review from a team July 9, 2020 10:08

This was referenced Jul 9, 2020

d/aws_prefix_list: add support for managed prefix lists #14110

Closed

New resource: aws_ec2_managed_prefix_list #14068

Merged

anGie44 force-pushed the master branch from 54db70d to 48ba2f7 Compare July 21, 2020 18:26

roberth-k added 4 commits July 21, 2020 21:35

d/aws_prefix_list: fix name filter override and lack of error when ma…

37c49d9

…tching multiple prefix lists

d/aws_prefix_list: sort cidrs

f259a03

d/aws_prefix_list: fix doc type and add example about finding a prefi…

3d2ca8e

…x list by name

d/aws_prefix_list: error check when setting cidr_blocks

a35c88c

roberth-k force-pushed the b-aws_prefix_list branch from 792047f to a35c88c Compare July 21, 2020 20:35

This was referenced Dec 2, 2020

ds/aws_prefix_list: Fix hardcoded regions #16521

Closed

tests/ds/region: Fix hardcoded region #16523

Merged

bflad added the breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. label Dec 3, 2020

bflad self-assigned this Dec 3, 2020

bflad added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Dec 3, 2020

bflad suggested changes Dec 3, 2020

View reviewed changes

roberth-k closed this Dec 13, 2020

ghost locked as resolved and limited conversation to collaborators Jan 12, 2021

roberth-k deleted the b-aws_prefix_list branch January 16, 2022 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

d/aws_prefix_list: fixes and enhancements #14109

d/aws_prefix_list: fixes and enhancements #14109

roberth-k commented Jul 9, 2020 •

edited

Loading

bflad left a comment

bflad Dec 3, 2020

bflad Dec 3, 2020

bflad Dec 3, 2020

bflad Dec 3, 2020

roberth-k commented Dec 13, 2020

ghost commented Jan 12, 2021

		case len(resp.PrefixLists) > 1:
		return fmt.Errorf("more than one prefix list matched the given set of criteria")

d/aws_prefix_list: fixes and enhancements #14109

d/aws_prefix_list: fixes and enhancements #14109

Conversation

roberth-k commented Jul 9, 2020 • edited Loading

Community Note

bflad left a comment

Choose a reason for hiding this comment

bflad Dec 3, 2020

Choose a reason for hiding this comment

bflad Dec 3, 2020

Choose a reason for hiding this comment

bflad Dec 3, 2020

Choose a reason for hiding this comment

bflad Dec 3, 2020

Choose a reason for hiding this comment

roberth-k commented Dec 13, 2020

ghost commented Jan 12, 2021

roberth-k commented Jul 9, 2020 •

edited

Loading